Skip to content

fix(spec): make what collectFlowGraphs RETURNS match its declared FlowNodeParsed[] - #16922

Merged
zhuangjianguo merged 4 commits into
mainfrom
claude/issue-16752-collectflowgraphs-nonrecord-members
Sep 8, 2026
Merged

fix(spec): make what collectFlowGraphs RETURNS match its declared FlowNodeParsed[]#16922
zhuangjianguo merged 4 commits into
mainfrom
claude/issue-16752-collectflowgraphs-nonrecord-members

Conversation

@claude

@claude claude Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Fixes #16752

collectFlowGraphs declares each FlowGraph.nodes as readonly FlowNodeParsed[] and then pushed the list verbatim. That list is one the walk picks up itself out of a container's open z.record config, after an Array.isArray that proves the LIST and never its MEMBERS — so an empty item in a YAML nodes: list under a loop body reached a returned graph as null. No coercion at a call site could prevent it, which is the ground on which triage routed this producer-side: 「无caller-side discipline 可以触及一个 producer 自己从开放 z.record 里挑出来的数组」.

Premise re-derived on today's origin/main before writing

The card was measured on 7c12e475. PR #16825 (#16134) landed since, so both halves were re-measured against the merge base:

the card's defect re-measured
1. the walk-time dereference (THREW … at regionSlotsOf) DISCHARGED. visit already carries const raw: unknown = node; if (raw === null || typeof raw !== 'object') return; before regionSlotsOf(node), crediting #16134. The card's reproduction no longer reproduces at any depth. ⛔ Not touched again.
2. the contract violation in what is RETURNED LIVE, and broader than the card states.

Driving the built dist/ over five non-record shapes and seven nestings, counting graphs whose nodes hold a non-record:

BEFORE  null  | d=0 badGraphs=1 | d=1 badGraphs=1 | d=2 badGraphs=1 | d=30 badGraphs=1 | d=31 badGraphs=1 | d=32 badGraphs=1 | d=33 badGraphs=0
AFTER   null  | d=0 badGraphs=0 | d=1 badGraphs=0 | d=2 badGraphs=0 | d=30 badGraphs=0 | d=31 badGraphs=0 | d=32 badGraphs=0 | d=33 badGraphs=0

Identical rows for undefined, a string, a number and an array. ⭐ The leak is at every depth, not only at the MAX_REGION_DEPTH ceiling the card identified — visit pushed nodes verbatim and merely skipped non-records while walking, so the ceiling is one route in and not the only one. d=33 reads 0 because the innermost region sits past the ceiling and is never visited at all.

The change

Four lines of production code in packages/spec/src/automation/control-flow.zod.ts:

  • Filter what is handed out. const kept = nodes.filter((node) => isRegionDict(node)); and push kept only when something was actually dropped, so a well-formed flow keeps array identity and allocates nothing — copy-on-write, as parseFlowNodeRegions already is.
  • Skip what is walked, through the same predicate. The inline three-clause test became if (!isRegionDict(node)) return;. One spelling for both, so the two cannot drift. This also stops admitting an array, which typeof raw === 'object' did.
  • path stays indexed over the RAW list. The forEach still runs over nodes, never kept: graph.path anchors a Zod issue where the author wrote the node, so a drop must not renumber the siblings behind it.

⛔ Not a looser signature. The declared input and FlowGraph's field types are untouched — widening was the direction #15793 refused on the anti-AI-error axis, and this is the opposite move: the producer keeps a promise it had already made. ⛔ Not a throw either: this walk runs inside FlowSchema's parse, where a TypeError escapes safeParse instead of becoming an issue, so the repair is a drop and a skip.

The RESIDUAL_THROWS ledger — re-measured per row, not assumed

The card said the two #16752 rows come out. Because defect 1's throw was already gone, which rows are still earned was measured before deleting any: the suite was run with all four rows still present against the rebuilt producer, and the assertion is exact in both directions, so it named them itself.

row before after disposition
flows[].nodes · null throws out of lintFlowPatterns throws out of lintFlowPatterns KEPT — still earned
flows[].nodes · undefined throws out of lintFlowPatterns throws out of lintFlowPatterns KEPT — still earned
flows[].nodes[].config.body.nodes · null throws out of lintFlowPatterns + validateStackExpressions throws out of nothing REMOVED
flows[].nodes[].config.body.nodes · undefined throws out of lintFlowPatterns + validateStackExpressions throws out of nothing REMOVED

The two kept rows are #16751's: lint-flow-patterns.ts reads flow.nodes itself at :1426, which a producer repair cannot reach. Its two graph.nodes casts at :456 / :1522 are now covered from the producer side, and the docblock is corrected to say exactly that, so the next reader knows what is left to repair there. The run with the rows still present:

Tests  2 failed | 326 passed (328)
 FAIL  flows[].nodes[].config.body.nodes > with null > throws out of no rule but the ones still filed as broken
 FAIL  flows[].nodes[].config.body.nodes > with undefined > throws out of no rule but the ones still filed as broken
AssertionError: expected [] to deeply equal [ 'lintFlowPatterns', …(1) ]

After removing exactly those two: Tests 328 passed (328).

Ablation — direction predicted before it was run

Predicted red: 15 hands out only records cases (5 shapes x 3 nestings) plus 5 anti-vacuity cases = 20 failures, with the safeParse, path and identity pins staying green because they pin properties the unfixed code also had.

Run from the committed state, mutation = the whole producer file put back to the merge-base content, restore under trap ... EXIT INT TERM using git checkout HEAD -- PATH (which resets index and tree; a bare git checkout -- would take the content back out of the polluted index):

pre-mutation   'const kept = nodes.filter': 1    'graphs.push({ scope, path, nodes, edges });': 0
post-mutation  'const kept = nodes.filter': 0    'graphs.push({ scope, path, nodes, edges });': 1
blob now=7a0539c4150a28642a186cffcb022976ae470142  head=91919189b678c513bbb64749468658538e000085

ABLATION_TEST_EXIT=1
 Test Files  1 failed (1)
      Tests  20 failed | 24 passed (44)

restore: blob back=91919189b678c513bbb64749468658538e000085 == head
         git diff HEAD  []      git status --porcelain  []

20 failed, exactly as predicted, and all 20 inside the new describe. Landing and restore are proven by anchor count and blob hash, never by an editor's exit code; every zero-hit grep above has its non-zero counterpart on the same corpus.

Verification — exit codes captured before any pipe

check exit
pnpm --filter @objectstack/spec test — 466 files / 13053 tests 0
pnpm --filter @objectstack/spec typecheck 0
pnpm --filter @objectstack/spec check:generated — all 15 generated artifacts up to date 0
pnpm --filter @objectstack/lint test — 102 files / 3581 tests 0
pnpm --filter @objectstack/lint typecheck 0
pnpm check:nul-bytes — 8367 text files, no raw control bytes 0
pnpm check:cross-package-test-inputs 0
pnpm check:test-source-alias 0
pnpm check:spec-parsed-alias 0
pnpm check:published-files 0
pnpm lint — whole repo, 6381 files, 0 errors / 0 warnings, at d02e7d4d4 0

pnpm lint is the whole population, not a narrowing: the file count is read from eslint's own --format json output, and this repo runs one eslint.config.mjs which the config's own measured comment records as never enabling type-aware linting for any file.

No widening. check:api-surface and check:export-origins are both green inside check:generated: no export added, no signature loosened, no surface movement. The new predicate reuses the existing module-local isRegionDict.

node scripts/pm/dispatch-gates.mjs derives 76 families for these four paths; the 12 above were run locally and the remaining 64 are CI's farm. The derivation was re-run after merging origin/main in, because the first answer was flagged STALE TREE and would have been a well-formed answer about a tree nobody is on; the merged derivation is byte-identical to it.

验收备注

Noted, not filed:

  • A dropped member renumbers its siblings inside graph.nodes. One consumer indexes it positionally: the duplicate-node-id rule in flow.zod.ts builds nodes[index] and a Zod issue path from that index. This is the same trade-off the four sibling repairs already made and that non-record-object-entry.test.ts records as accepted — 「a difference in the path, never in whether the object was judged」 — and it only arises inside a list that was already malformed, where the region carries a schema refusal of its own. graph.path, which is the anchor that matters, is pinned to stay raw-indexed. Not filed as a defect because the repo has already ruled on this exact shape; recorded here because no PR currently queued would otherwise carry it. Successor: none queued.
  • The defensive (node as ...)?.id read in that same rule is now belt-and-braces for the null case, since graph.nodes holds only records. Dead-code tidying, deliberately not done here.
  • control-flow.zod.ts's docblock (and the reference page generated from it) says the schema and validateControlFlow "cannot fight" and meet at ONE seam — #16134 makes both sentences false #16835 is unblocked and untouched. It owns the two module-docblock sentences at :61 ("cannot fight") and :63 ("one seam"); both are byte-identical to the merge base, and nothing in this diff sits above them, so their line numbers have not moved either.
  • lint: two more flow-node-list readers throw on a non-record member — lintFlowPatterns and collectFlowVariableNames #16751 remains open and is not addressed here — its two kept ledger rows are the live record of that.

🤖 Generated with Claude Code


Generated by Claude Code


Clause-② declaration — added by the domain:spec execution seat, 2026-09-08T16:4xZ

Check Changeset refused this PR because the level axis was NOT MEASURED: a patch sits on @objectstack/spec, whose packages/*/src/** this diff moves, and no declaration was readable. The remedy the gate names is the declaration, ⛔ never dropping or regrading the changeset. Declaring it here, in the fixed spelling, as the claiming seat:

Clause-②: no
— this PR narrows what a published function hands out so that it matches the type it already
declares. It puts no new key on a published payload, adds no export (it reuses the existing
module-local isRegionDict), and moves no accept set: FlowSchema accepts and refuses exactly the
documents it did before, because the repair is drop-and-skip inside the parse rather than a new refusal.
check:generated is green across all 15 artifacts, check:api-surface and check:export-origins
included. ⇒ 拉回已声明契约, so the patch above stands.

The published contract text this no is required to quote

A no that deletes a refusal must cite the published text that already denied it — 「删已发布契约文本本就否定的误拒本身是 no;该 no 须引那段文本,缺引即读作缺申报」. This PR removes two throws. They were never verdicts:

① The declared return type. collectFlowGraphs declares FlowNodeParsed[]. Handing out a null member was already denied by that declaration — the defect is that the implementation did it anyway, at every nesting depth.

② The ledger that holds those throws says so itself, in its own docblock on packages/lint/src/non-record-object-entry.test.ts — and it names this card:

flows[].nodes[].config.body.nodes / validateStackExpressions + lintFlowPatterns (#16752) — neither rule's own reader is at fault here: both throw from INSIDE collectFlowGraphs, whose region walk reads node.config off a member of an inner list it checked only with Array.isArray. No coercion at either call site reaches that list, which is why #15793 stopped and filed the fork instead of widening a packages/spec contract to tolerate malformed members.

and, on removal being the expected outcome of the repair rather than a contract change:

a residual that gets fixed reds because its row is now a lie and has to go

⇒ The two removed rows are catalogued defects with a filed card, not designed refusals; and the alternative that would have been a clause-② yeswidening a packages/spec contract to tolerate malformed members — is precisely the direction #15793 rejected and this PR does not take.

⚠️ Stated openly rather than buried, because it is the one thing that could flip this reading: filtering graph.nodes renumbers surviving members inside that array, and flow.zod.ts's duplicate-node-id rule indexes it positionally. graph.path — the machine-consumed anchor — is kept raw-indexed and pinned by a test that stays green under ablation. The shift is confined to the nodes[i] label inside a list that was already malformed, and is the same trade-off the four sibling repairs made and that non-record-object-entry.test.ts already records as accepted. If a reviewer reads that renumbering as an accept-set movement, this declaration flips to yes and the PR parks; ⛔ nothing here was flipped to ready, enqueued, or given auto-merge.


Generated by Claude Code

… out

`collectFlowGraphs` declares each `FlowGraph.nodes` as `readonly
FlowNodeParsed[]`, then pushed the list verbatim. That list is one the walk
picks up ITSELF out of a container's open `z.record` config, after an
`Array.isArray` that proves the LIST and never its MEMBERS — so an empty YAML
list item under a `loop` body reached a returned graph as `null`, at every
depth, and no coercion at a call site could reach it.

Filter what is handed out and skip what is walked through one predicate
(`isRegionDict`), preserving array identity when nothing is dropped. The walk
runs inside `FlowSchema`'s parse, so this stays a drop and a skip: a throw here
would escape `safeParse` rather than become an issue. `path` stays indexed over
the raw list, so a Zod issue is still anchored where the author wrote the node.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016N6xmWt5hYm94ffVEwGH8x
…ucer fix earned

Re-measured with all four rows still present: the two
`flows[].nodes[].config.body.nodes` rows went red demanding a throw that no
longer happens, while both `flows[].nodes` rows stayed green — those are
`lint-flow-patterns.ts` reading `flow.nodes` itself, untouched by a producer
repair. Remove the two that stopped being earned, keep the two that did not,
and correct the prose that described the retired pair as live.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016N6xmWt5hYm94ffVEwGH8x
@github-actions github-actions Bot added the size/m label Sep 8, 2026
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

2 anchor(s) derived from 1 changed package(s); no hand-written page names any of them, so this run has nothing to listnot a clean bill of health. This check sees only pages that NAME a derived anchor: one that documents this change in prose, or enumerates it in an authoring dialect, names none and stays invisible to it on every run.

What this run could not see
  • the SDK route bridge reached 60 of 216 client-bound route-ledger rows — the other 156 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 156: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 100 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 131 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 70f7d6d735505c03a80bdb279262af5aa7c77ff1packageMentionDocs.

Which tree this was computed on

This run read content/docs from d93df704088f4288a7c623985bd58e010e17f8dc — the merge of head d02e7d4d4c11ba1c360596bc3fdc6315add553db into base 70f7d6d735505c03a80bdb279262af5aa7c77ff1, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin d93df704088f4288a7c623985bd58e010e17f8dc && git checkout d93df704088f4288a7c623985bd58e010e17f8dc
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 70f7d6d735505c03a80bdb279262af5aa7c77ff1 d02e7d4d4c11ba1c360596bc3fdc6315add553db && git checkout -B drift-repro 70f7d6d735505c03a80bdb279262af5aa7c77ff1 && git merge --no-ff d02e7d4d4c11ba1c360596bc3fdc6315add553db

node scripts/docs-audit/affected-docs.mjs --json 70f7d6d735505c03a80bdb279262af5aa7c77ff1

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

@claude

claude Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

ACCEPT — domain:spec execution seat, 2026-09-08T16:58Z

Reviewed against head d02e7d4d4, base 0939ba008 (merge base, measured — ⛔ not a two-dot diff). Where I rely on the dev's measurement rather than my own, I say so.

What this seat measured itself

reading value instrument
diff shape 4 files, +181 / −29 git diff --stat 0939ba008 d02e7d4d4
api-surface / export-origins touched 0 files git diff --name-only … | grep -E "api-surface|export-origins"
clause-② pair exit 0 — declaration legible, both carriers agree, no widening tell check-clause2-carriers.mjs --pair 16922
governed surface 0 of 4 paths hit the register, re-run on the FINAL file list check-governed-merges.mjs --test
checks 34 names, 0 running, 0 non-green, mergeable_state: clean aggregated by name on the PR-object head
model tier 160 harness-stamped "model":"claude-opus-5", no other value subagent transcript grep — ⛔ never a self-description

The ledger movement, verified against origin/main rather than taken from the report:

rows on origin/main on the head
flows[].nodes[].config.body.nodes · … 2 0 — earned out by this repair
flows[].nodes · … 2 2 — survive, correctly

The two that survive are #16751's own flow.nodes read at lint-flow-patterns.ts:1426, which no producer repair reaches. Both dispositions came out of running the suite with all four rows present and letting the both-directions assertion name them — ⛔ not from reasoning about which ought to go.

The landed shape is one predicate, not a fifth spelling of the record test. isRegionDict (declared :495) is now the single reader at :671, :761 (const kept = nodes.filter((node) => isRegionDict(node)) — the handout), :769 and :771 (the walk). That is what makes the returned value and the walked value agree by construction rather than by two tests that could drift.

The clause-② no, and the citation it is required to carry

Clause-②: no is declared in the PR body. A no that deletes a refusal must quote the published text that already denied it — 「删已发布契约文本本就否定的误拒本身是 no;该 no 须引那段文本,缺引即读作缺申报」. Two texts do:

  1. The declared return type. collectFlowGraphs declares FlowNodeParsed[]; handing out a null member was already denied by that declaration.
  2. The ledger's own docblock, which names this card and frames its rows as defects, ⛔ not verdicts:

flows[].nodes[].config.body.nodes … (#16752) — neither rule's own reader is at fault here: both throw from INSIDE collectFlowGraphsNo coercion at either call site reaches that list, which is why #15793 stopped and filed the fork instead of widening a packages/spec contract to tolerate malformed members.

and, on removal being the expected outcome:

a residual that gets fixed reds because its row is now a lie and has to go

⭐ The alternative that would have been a clause-② yes — widening a packages/spec contract to tolerate malformed members — is precisely the direction #15793 rejected, and this PR takes the opposite one. FlowSchema accepts and refuses exactly the documents it did before, because the repair is drop-and-skip inside the parse rather than a new refusal.

The dev's own measurements, credited as its readings

  • Premise re-derived on origin/main before writing, and it came out broader than the card: defect 1 (the walk-time dereference) is discharged by Decision: do a flow's top-level nodes[] and its region bodies (loop / try_catch / parallel) share ONE node-id space, or two? — uniqueness is now enforced inside each, never across #16134 and was left alone; defect 2 held at every nesting depth (badGraphs=1 at 0, 1, 2, 30, 31, 32 across all five non-record shapes), not only at the MAX_REGION_DEPTH ceiling the card identified.
  • Ablation with the direction predicted before running, landing proven by anchor count and blob hash rather than an editor exit code: 20 predicted failures, 20 failed / 24 passed, all inside the new describe; restored to an empty git diff HEAD and empty git status --porcelain.
  • pnpm lint over the whole repo — 6381 files, 0 errors, 0 warnings — a full population, ⛔ not a narrowing.
  • check:generated = 0 with check:api-surface and check:export-origins both green — the no-widening evidence.
  • ⚠️ Its first gate derivation was flagged STALE TREE (branch 2 commits behind); it merged origin/main and re-derived, and the merged answer was byte-identical.

⭐ The consequence it did not hide

Filtering graph.nodes renumbers surviving members, and flow.zod.ts's duplicate-node-id rule indexes that array positionally. graph.path — the machine-consumed anchor — is kept raw-indexed and pinned by a test that stays green under ablation deliberately, because it pins a property the unfixed code also had. The shift is confined to the nodes[i] label inside a list that was already malformed and already carries its own region-schema refusal, and it is the same trade-off the four sibling repairs made and that non-record-object-entry.test.ts already records as accepted. It is called out in the PR's ## 验收备注 rather than filed, because the repo has ruled on this exact shape — and because no queued PR would otherwise carry it.

⚠️ What I did not verify myself

The ablation and the ledger re-measure are the dev's readings, not mine: reproducing them requires editing a tree, and ⛔ this seat never writes code. What I did instead was verify their outcome on the two trees — the row counts above are mine, taken from origin/main and the PR head.

check:dual-build-cjs-loads and check:type-check-debt are NOT MEASURED locally (exit 3, PREREQUISITE NOT MET — both need the whole workspace built). CI builds that closure, so the PR's own check set governs.

Landing

Every check reads completed with success/skipped aggregated by name, mergeable_state is clean, the clause-② pair reads 0 and the surface is not governed. ⇒ Flipping ready, re-verifying after the ready_for_review re-triggered gates close, then enqueuing. Enqueue is confirmed on the added_to_merge_queue timeline event, ⛔ never on auto_merge (null on this repo even after a successful enable).

Also noted for whoever reads this later: #16835 is unblocked by this landing and untouched — it owns the module docblock sentences at :61 and :63, both byte-identical to the merge base, with nothing in this diff sitting above them, so their line numbers have not moved either.


Generated by Claude Code

@zhuangjianguo
zhuangjianguo marked this pull request as ready for review September 8, 2026 16:59
@zhuangjianguo
zhuangjianguo added this pull request to the merge queue Sep 8, 2026
Merged via the queue into main with commit ae05f2e Sep 8, 2026
41 of 42 checks passed
@zhuangjianguo
zhuangjianguo deleted the claude/issue-16752-collectflowgraphs-nonrecord-members branch September 8, 2026 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/m tests tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

spec: collectFlowGraphs dereferences a non-record member of a NESTED region's node list — its own walk, not the caller's

2 participants